-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix NativeAOT tracing tests to work with both CoreCLR and NativeAOT #123553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: mdh1418 <16830051+mdh1418@users.noreply.github.com>
Co-authored-by: mdh1418 <16830051+mdh1418@users.noreply.github.com>
Co-authored-by: mdh1418 <16830051+mdh1418@users.noreply.github.com>
…ling to actually tail call (#123513) call.tail in the interpreter isn't guaranteed to tail-call in some cases, so we need to put in a ret instruction after the tail-call to ensure that the runtime doesn't execute invalid code. The code for doing a ret is extracted into a helper routine, and now called from both the CEE_JMP and CEE_RET pathways. The only change made to the code was to unify where the ip adjustment was to not happen in the EmitRet logic and instead keep it all in the same place in CEE_RET handling case. This fixes these test cases on Windows Arm64 JIT/Directed/pinvoke/jump JIT/Directed/pinvoke/tail_pinvoke
These are all crashing: ``` 11:31:58.741 Running test: tracing/userevents/custommetadata/custommetadata/custommetadata.cmd Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'path1') at System.ArgumentNullException.Throw(String) at System.IO.Path.Combine(String, String, String) at Tracing.UserEvents.Tests.Common.UserEventsTestRunner.ResolveRecordTracePath(String) at Tracing.UserEvents.Tests.Common.UserEventsTestRunner.RunOrchestrator(String, String, Func`2, Int32, Int32) at Tracing.UserEvents.Tests.CustomMetadata.CustomMetadata.Main(String[] args) ```
Test started running with #123112 Cc @dotnet/ilc-contrib
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@copilot on NativeAOT, it looks like the runtime tests layout is a bit different as they will embed an extra Instead of using Afterwards can you run the |
…OT compatibility Co-authored-by: mdh1418 <16830051+mdh1418@users.noreply.github.com>
Updated to use |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Failing without NAOT: |
|
Detect runtime type by checking if Assembly.Location is empty: - CoreCLR: Use Process.GetCurrentProcess().MainModule.FileName (corerun) with assembly path - NativeAOT: Use Environment.ProcessPath (native executable) with AppContext.BaseDirectory This allows tests to run successfully in both scenarios: - CoreCLR: corerun <path-to-dll> tracee - NativeAOT: <native-exe> tracee Co-authored-by: mdh1418 <16830051+mdh1418@users.noreply.github.com>
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
Remove redundant fallback logic since runtime detection is now handled in RunOrchestrator. The userEventsScenarioDir parameter is always valid at this point. Co-authored-by: mdh1418 <16830051+mdh1418@users.noreply.github.com>
Reverted to original code and implemented conditional logic to support both runtimes. Changes in commits 3ed348e and 54e7d86:
Tests now work in both scenarios without requiring code changes to test files. |
Replace null-forgiving operators with proper null checks and throw meaningful exceptions if paths cannot be determined. Co-authored-by: mdh1418 <16830051+mdh1418@users.noreply.github.com>
| } | ||
| else | ||
| { | ||
| traceeStartInfo.FileName = Process.GetCurrentProcess().MainModule?.FileName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Process.GetCurrentProcess().MainModule?.FileName should be same as Environment.ProcessPath. Change this to use Environment.ProcessPath for all runtime flavors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use Environment.ProcessPath for all runtime flavors in commit bc9c75c. This simplifies the code since both should return the same value.
Simplify tracee execution by using Environment.ProcessPath for both CoreCLR and NativeAOT instead of Process.GetCurrentProcess().MainModule.FileName since they should be equivalent. Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
| string recordTracePath = ResolveRecordTracePath(userEventsScenarioDir); | ||
| string scriptFilePath = Path.Combine(userEventsScenarioDir, $"{scenarioName}.script"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Both the Record-Trace executable and scenario's script still need to be resolved according todepending on isNativeAot.
Can you make ResolveRecordTracePath
string usereventsRoot;
if (isNativeAot)
{
// scenario dir: .../tracing/userevents/<scenario>/<scenario>/native/
usereventsRoot = Path.GetFullPath(Path.Combine(userEventsScenarioDir, "..", "..", ".."));
}
else
{
// scenario dir: .../tracing/userevents/<scenario>/<scenario>
usereventsRoot = Path.GetFullPath(Path.Combine(userEventsScenarioDir, "..", ".."));
}
// common dir: .../tracing/userevents/common/userevents_common
string commonDir = Path.Combine(usereventsRoot, "common", "userevents_common");
string recordTracePath = Path.Combine(commonDir, "record-trace");
return recordTracePath;And can you make another helper for resolving scriptFilePath with resolving path logic dependent on isNativeAot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated path resolution to handle NativeAOT's native/ subdirectory in commit 3a50161:
- Modified
ResolveRecordTracePathto navigate up 3 levels for NativeAOT vs 2 for CoreCLR - Added
ResolveScriptFilePathhelper that navigates up 1 level for NativeAOT to find script - Both methods now use
isNativeAotparameter for conditional logic
NativeAOT tests have executables in a native/ subdirectory, so paths need to navigate up an extra level. Updated ResolveRecordTracePath to handle both structures and added ResolveScriptFilePath helper method. - NativeAOT: userEventsScenarioDir is .../scenario/scenario/native/ - CoreCLR: userEventsScenarioDir is .../scenario/scenario/ - Script and record-trace paths now resolve correctly for both Co-authored-by: mdh1418 <16830051+mdh1418@users.noreply.github.com>
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
NativeAOT tracing tests in
src/tests/tracing/userevents/*crash withArgumentNullExceptioninResolveRecordTracePathbecauseAssembly.Locationreturns empty string in NativeAOT builds. Additionally, NativeAOT has a different directory layout with executables placed in anative/subdirectory, and NativeAOT tests run executables directly without using corerun.This PR fixes the tests to work with both CoreCLR and NativeAOT by implementing runtime detection and using conditional logic for path resolution and tracee execution. It reverts PR #123541 which had marked these tests as NativeAOT incompatible.
Changes Made
Assembly.Location(passed astraceeAssemblyPath) returns empty stringPath.GetDirectoryName(traceeAssemblyPath)to get scenario directory (.../scenario/scenario/)AppContext.BaseDirectorywhich points to the scenario directory withnative/subdirectory (.../scenario/scenario/native/)ResolveRecordTracePathto acceptisNativeAotparameter and navigate up 3 levels for NativeAOT (fromnative/subdirectory) vs 2 levels for CoreCLRResolveScriptFilePathhelper method that navigates up 1 level for NativeAOT to find script in parent directory, while CoreCLR finds it in current directoryEnvironment.ProcessPathfor both CoreCLR and NativeAOT (returns corerun for CoreCLR, native executable for NativeAOT)<assembly-path> traceetraceeonlyDirectory.Build.propsthat marked tests asNativeAotIncompatibletraceeAssemblyPathparameter so test files remain unchangedTechnical Notes
The solution detects the runtime by checking if
traceeAssemblyPathis empty:Assembly.Locationreturns valid path,traceeAssemblyPathis populatedAssembly.Locationreturns empty string,traceeAssemblyPathis emptyPath resolution handles different directory structures:
userEventsScenarioDiris.../tracing/userevents/<scenario>/<scenario>/(derived fromPath.GetDirectoryName()of assembly location)userEventsScenarioDiris.../tracing/userevents/<scenario>/<scenario>/native/(fromAppContext.BaseDirectory)Record-trace executable resolution:
native/subdirectory to reach userevents root.../tracing/userevents/common/userevents_common/record-traceScript file resolution:
.../scenario/scenario/<scenario>.script)native/subdirectory to find script (.../scenario/scenario/<scenario>.script)Tracee execution uses
Environment.ProcessPathfor both runtimes:Environment.ProcessPathreturns corerun path, runscorerun <path-to-scenario.dll> traceeEnvironment.ProcessPathreturns native executable path, runs<native-executable> traceeAll test files continue to pass
Assembly.Locationunchanged, which triggers the correct conditional logic based on the runtime.TMPDIR is already configured correctly, but diagnostic port discovery will still fail for NativeAOT due to the perfmap dependency (see microsoft/one-collect#226).
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.